[lld-macho] Use larger ordinal encoding if import count requires it #98305

BertalanD · 2024-07-10T11:26:19Z

The default dyld_chained_import entry format only allocates 8 bits to the library ordinal, of which values 0xF1 - 0xFF are reserved for special ordinals (BIND_SPECIAL_DYLIB_* values). If there are more than 240 imported dylibs (as in the case of component builds of Chromium), we need to switch to dyld_chained_import_addend64, which stores 16-bit ordinals.

The default `dyld_chained_import` entry format only allocates 8 bits to the library ordinal, of which values 0xF1 - 0xFF are reserved for special ordinals (`BIND_SPECIAL_DYLIB_*` values). If there are more than 240 imported dylibs (as in the case of component builds of Chromium), we need to switch to `dyld_chained_import_addend64`, which stores 16-bit offsets.

llvmbot · 2024-07-10T11:26:51Z

@llvm/pr-subscribers-lld

Author: Daniel Bertalan (BertalanD)

Changes

The default dyld_chained_import entry format only allocates 8 bits to the library ordinal, of which values 0xF1 - 0xFF are reserved for special ordinals (BIND_SPECIAL_DYLIB_* values). If there are more than 240 imported dylibs (as in the case of component builds of Chromium), we need to switch to dyld_chained_import_addend64, which stores 16-bit offsets.

Full diff: https://github.com/llvm/llvm-project/pull/98305.diff

1 Files Affected:

(modified) lld/MachO/SyntheticSections.cpp (+18-15)

diff --git a/lld/MachO/SyntheticSections.cpp b/lld/MachO/SyntheticSections.cpp
index 6a7284fefcf29..24255d477bbb2 100644
--- a/lld/MachO/SyntheticSections.cpp
+++ b/lld/MachO/SyntheticSections.cpp
@@ -544,6 +544,14 @@ static void flushOpcodes(const BindIR &op, raw_svector_ostream &os) {
   }
 }
 
+static bool needsWeakBind(const Symbol &sym) {
+  if (auto *dysym = dyn_cast<DylibSymbol>(&sym))
+    return dysym->isWeakDef();
+  if (auto *defined = dyn_cast<Defined>(&sym))
+    return defined->isExternalWeakDef();
+  return false;
+}
+
 // Non-weak bindings need to have their dylib ordinal encoded as well.
 static int16_t ordinalForDylibSymbol(const DylibSymbol &dysym) {
   if (config->namespaceKind == NamespaceKind::flat || dysym.isDynamicLookup())
@@ -553,6 +561,8 @@ static int16_t ordinalForDylibSymbol(const DylibSymbol &dysym) {
 }
 
 static int16_t ordinalForSymbol(const Symbol &sym) {
+  if (config->emitChainedFixups && needsWeakBind(sym))
+    return BIND_SPECIAL_DYLIB_WEAK_LOOKUP;
   if (const auto *dysym = dyn_cast<DylibSymbol>(&sym))
     return ordinalForDylibSymbol(*dysym);
   assert(cast<Defined>(&sym)->interposable);
@@ -2276,14 +2286,6 @@ bool ChainedFixupsSection::isNeeded() const {
   return true;
 }
 
-static bool needsWeakBind(const Symbol &sym) {
-  if (auto *dysym = dyn_cast<DylibSymbol>(&sym))
-    return dysym->isWeakDef();
-  if (auto *defined = dyn_cast<Defined>(&sym))
-    return defined->isExternalWeakDef();
-  return false;
-}
-
 void ChainedFixupsSection::addBinding(const Symbol *sym,
                                       const InputSection *isec, uint64_t offset,
                                       int64_t addend) {
@@ -2312,7 +2314,7 @@ ChainedFixupsSection::getBinding(const Symbol *sym, int64_t addend) const {
   return {it->second, 0};
 }
 
-static size_t writeImport(uint8_t *buf, int format, uint32_t libOrdinal,
+static size_t writeImport(uint8_t *buf, int format, int16_t libOrdinal,
                           bool weakRef, uint32_t nameOffset, int64_t addend) {
   switch (format) {
   case DYLD_CHAINED_IMPORT: {
@@ -2430,11 +2432,8 @@ void ChainedFixupsSection::writeTo(uint8_t *buf) const {
   uint64_t nameOffset = 0;
   for (auto [import, idx] : bindings) {
     const Symbol &sym = *import.first;
-    int16_t libOrdinal = needsWeakBind(sym)
-                             ? (int64_t)BIND_SPECIAL_DYLIB_WEAK_LOOKUP
-                             : ordinalForSymbol(sym);
-    buf += writeImport(buf, importFormat, libOrdinal, sym.isWeakRef(),
-                       nameOffset, import.second);
+    buf += writeImport(buf, importFormat, ordinalForSymbol(sym),
+                       sym.isWeakRef(), nameOffset, import.second);
     nameOffset += sym.getName().size() + 1;
   }
 
@@ -2459,7 +2458,11 @@ void ChainedFixupsSection::finalizeContents() {
     error("cannot encode chained fixups: imported symbols table size " +
           Twine(symtabSize) + " exceeds 4 GiB");
 
-  if (needsLargeAddend || !isUInt<23>(symtabSize))
+  bool needsLargeOrdinal = any_of(bindings, [](const auto &p) {
+    return ordinalForSymbol(*p.first.first) > 0xF0;
+  });
+
+  if (needsLargeAddend || !isUInt<23>(symtabSize) || needsLargeOrdinal)
     importFormat = DYLD_CHAINED_IMPORT_ADDEND64;
   else if (needsAddend)
     importFormat = DYLD_CHAINED_IMPORT_ADDEND;

llvmbot · 2024-07-10T11:26:51Z

@llvm/pr-subscribers-lld-macho

Author: Daniel Bertalan (BertalanD)

Changes

The default dyld_chained_import entry format only allocates 8 bits to the library ordinal, of which values 0xF1 - 0xFF are reserved for special ordinals (BIND_SPECIAL_DYLIB_* values). If there are more than 240 imported dylibs (as in the case of component builds of Chromium), we need to switch to dyld_chained_import_addend64, which stores 16-bit offsets.

Full diff: https://github.com/llvm/llvm-project/pull/98305.diff

1 Files Affected:

(modified) lld/MachO/SyntheticSections.cpp (+18-15)

diff --git a/lld/MachO/SyntheticSections.cpp b/lld/MachO/SyntheticSections.cpp
index 6a7284fefcf29..24255d477bbb2 100644
--- a/lld/MachO/SyntheticSections.cpp
+++ b/lld/MachO/SyntheticSections.cpp
@@ -544,6 +544,14 @@ static void flushOpcodes(const BindIR &op, raw_svector_ostream &os) {
   }
 }
 
+static bool needsWeakBind(const Symbol &sym) {
+  if (auto *dysym = dyn_cast<DylibSymbol>(&sym))
+    return dysym->isWeakDef();
+  if (auto *defined = dyn_cast<Defined>(&sym))
+    return defined->isExternalWeakDef();
+  return false;
+}
+
 // Non-weak bindings need to have their dylib ordinal encoded as well.
 static int16_t ordinalForDylibSymbol(const DylibSymbol &dysym) {
   if (config->namespaceKind == NamespaceKind::flat || dysym.isDynamicLookup())
@@ -553,6 +561,8 @@ static int16_t ordinalForDylibSymbol(const DylibSymbol &dysym) {
 }
 
 static int16_t ordinalForSymbol(const Symbol &sym) {
+  if (config->emitChainedFixups && needsWeakBind(sym))
+    return BIND_SPECIAL_DYLIB_WEAK_LOOKUP;
   if (const auto *dysym = dyn_cast<DylibSymbol>(&sym))
     return ordinalForDylibSymbol(*dysym);
   assert(cast<Defined>(&sym)->interposable);
@@ -2276,14 +2286,6 @@ bool ChainedFixupsSection::isNeeded() const {
   return true;
 }
 
-static bool needsWeakBind(const Symbol &sym) {
-  if (auto *dysym = dyn_cast<DylibSymbol>(&sym))
-    return dysym->isWeakDef();
-  if (auto *defined = dyn_cast<Defined>(&sym))
-    return defined->isExternalWeakDef();
-  return false;
-}
-
 void ChainedFixupsSection::addBinding(const Symbol *sym,
                                       const InputSection *isec, uint64_t offset,
                                       int64_t addend) {
@@ -2312,7 +2314,7 @@ ChainedFixupsSection::getBinding(const Symbol *sym, int64_t addend) const {
   return {it->second, 0};
 }
 
-static size_t writeImport(uint8_t *buf, int format, uint32_t libOrdinal,
+static size_t writeImport(uint8_t *buf, int format, int16_t libOrdinal,
                           bool weakRef, uint32_t nameOffset, int64_t addend) {
   switch (format) {
   case DYLD_CHAINED_IMPORT: {
@@ -2430,11 +2432,8 @@ void ChainedFixupsSection::writeTo(uint8_t *buf) const {
   uint64_t nameOffset = 0;
   for (auto [import, idx] : bindings) {
     const Symbol &sym = *import.first;
-    int16_t libOrdinal = needsWeakBind(sym)
-                             ? (int64_t)BIND_SPECIAL_DYLIB_WEAK_LOOKUP
-                             : ordinalForSymbol(sym);
-    buf += writeImport(buf, importFormat, libOrdinal, sym.isWeakRef(),
-                       nameOffset, import.second);
+    buf += writeImport(buf, importFormat, ordinalForSymbol(sym),
+                       sym.isWeakRef(), nameOffset, import.second);
     nameOffset += sym.getName().size() + 1;
   }
 
@@ -2459,7 +2458,11 @@ void ChainedFixupsSection::finalizeContents() {
     error("cannot encode chained fixups: imported symbols table size " +
           Twine(symtabSize) + " exceeds 4 GiB");
 
-  if (needsLargeAddend || !isUInt<23>(symtabSize))
+  bool needsLargeOrdinal = any_of(bindings, [](const auto &p) {
+    return ordinalForSymbol(*p.first.first) > 0xF0;
+  });
+
+  if (needsLargeAddend || !isUInt<23>(symtabSize) || needsLargeOrdinal)
     importFormat = DYLD_CHAINED_IMPORT_ADDEND64;
   else if (needsAddend)
     importFormat = DYLD_CHAINED_IMPORT_ADDEND;

int3

lgtm!

nico

Should we add things like assert(libOrdinal <= 255) to writeImport() (and similar for nameOffset, for all 3 struct types)? (Not necessarily in this PR)

BertalanD · 2024-07-10T16:31:01Z

Should we add things like assert(libOrdinal <= 255) to writeImport() (and similar for nameOffset, for all 3 struct types)? (Not necessarily in this PR)

Sure, I can add those. I'll submit it as a separate PR tomorrow as I'm not on my main dev machine right now.

…lvm#98305) The default `dyld_chained_import` entry format only allocates 8 bits to the library ordinal, of which values 0xF1 - 0xFF are reserved for special ordinals (`BIND_SPECIAL_DYLIB_*` values). If there are more than 240 imported dylibs (as in the case of component builds of Chromium), we need to switch to `dyld_chained_import_addend64`, which stores 16-bit ordinals.

This reverts commit f55b79f. The known issues with chained fixups have been addressed by llvm#98913, llvm#98305, llvm#97156 and llvm#95171. Compared to the original commit, support for xrOS (which postdates chained fixups' introduction) was added and an unnecessary test change was removed. ---------- Original commit message: Enable chained fixups in lld when all platform and version criteria are met. This is an attempt at simplifying the logic used in ld 907: https://github.com/apple-oss-distributions/ld64/blob/93d74eafc37c0558b4ffb88a8bc15c17bed44a20/src/ld/Options.cpp#L5458-L5549 Some changes were made to simplify the logic: - only enable chained fixups for macOS from 13.0 to avoid the arch check - only enable chained fixups for iphonesimulator from 16.0 to avoid the arch check - don't enable chained fixups for not specifically listed platforms - don't enable chained fixups for arm64_32

This reverts commit f55b79f. The known issues with chained fixups have been addressed by #98913, #98305, #97156 and #95171. Compared to the original commit, support for xrOS (which postdates chained fixups' introduction) was added and an unnecessary test change was removed. ---------- Original commit message: Enable chained fixups in lld when all platform and version criteria are met. This is an attempt at simplifying the logic used in ld 907: https://github.com/apple-oss-distributions/ld64/blob/93d74eafc37c0558b4ffb88a8bc15c17bed44a20/src/ld/Options.cpp#L5458-L5549 Some changes were made to simplify the logic: - only enable chained fixups for macOS from 13.0 to avoid the arch check - only enable chained fixups for iphonesimulator from 16.0 to avoid the arch check - don't enable chained fixups for not specifically listed platforms - don't enable chained fixups for arm64_32

Summary: This reverts commit f55b79f. The known issues with chained fixups have been addressed by #98913, #98305, #97156 and #95171. Compared to the original commit, support for xrOS (which postdates chained fixups' introduction) was added and an unnecessary test change was removed. ---------- Original commit message: Enable chained fixups in lld when all platform and version criteria are met. This is an attempt at simplifying the logic used in ld 907: https://github.com/apple-oss-distributions/ld64/blob/93d74eafc37c0558b4ffb88a8bc15c17bed44a20/src/ld/Options.cpp#L5458-L5549 Some changes were made to simplify the logic: - only enable chained fixups for macOS from 13.0 to avoid the arch check - only enable chained fixups for iphonesimulator from 16.0 to avoid the arch check - don't enable chained fixups for not specifically listed platforms - don't enable chained fixups for arm64_32 Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251071

llvmbot added lld lld:MachO labels Jul 10, 2024

add comment

f3a71e5

int3 approved these changes Jul 10, 2024

View reviewed changes

nico approved these changes Jul 10, 2024

View reviewed changes

nico reviewed Jul 10, 2024

View reviewed changes

BertalanD merged commit 3c8b18b into llvm:main Jul 10, 2024
7 checks passed

BertalanD mentioned this pull request Jul 16, 2024

Reapply "[lld] enable fixup chains by default (#79894)" #99255

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[lld-macho] Use larger ordinal encoding if import count requires it #98305

[lld-macho] Use larger ordinal encoding if import count requires it #98305

Uh oh!

BertalanD commented Jul 10, 2024 •

edited

Loading

Uh oh!

llvmbot commented Jul 10, 2024

Uh oh!

llvmbot commented Jul 10, 2024

Uh oh!

int3 left a comment

Uh oh!

nico left a comment •

edited

Loading

Uh oh!

BertalanD commented Jul 10, 2024

Uh oh!

Uh oh!

Uh oh!

[lld-macho] Use larger ordinal encoding if import count requires it #98305

[lld-macho] Use larger ordinal encoding if import count requires it #98305

Uh oh!

Conversation

BertalanD commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 10, 2024

Uh oh!

llvmbot commented Jul 10, 2024

Uh oh!

int3 left a comment

Choose a reason for hiding this comment

Uh oh!

nico left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BertalanD commented Jul 10, 2024

Uh oh!

Uh oh!

Uh oh!

BertalanD commented Jul 10, 2024 •

edited

Loading

nico left a comment •

edited

Loading